replace pure annotations in promotion with inline #21771

andyferris · 2017-05-10T11:58:00Z

It seems to me that promote_type is not "hyperpure" since promote_rule is designed to be changed by the user at any time. However, constant propagation and inlining seem to be powerful enough to handle promote_rule, so that's what I've implemented here. Perhaps some inlines are unnecessary.

I haven't had time to test this thoroughly for inference/performance (hence the WIP) but I'm having trouble judging if this is a terrible idea for some reason (hence the RFC). Nanosoldier might shed some light...

ref: https://discourse.julialang.org/t/limitations-to-the-fix-of-265/3613

KristofferC · 2017-05-10T12:02:57Z

@nanosoldier runbenchmarks(ALL, vs = ":master")

nanosoldier · 2017-05-10T12:28:07Z

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @jrevels

vtjnash · 2017-05-10T20:49:19Z

It's a good change. Need to figure out the CI failures, then this should be merged.

JeffBezanson · 2017-05-10T21:10:09Z

base/promotion.jl

-promote_type()  = (@_pure_meta; Bottom)
-promote_type(T) = (@_pure_meta; T)
-promote_type(T, S, U, V...) = (@_pure_meta; promote_type(T, promote_type(S, U, V...)))
+promote_type()  = (@_inline_meta; Bottom)


Is there any reason to put the inline annotation here?

Nope - so far it's been mostly an exercise of find-replace. I just wanted to quickly get it working and check with Jameson if there was a reason not to do this, but I'll tidy it up soon.

andyferris · 2017-05-13T06:54:08Z

Hilariously, this breaks inference on promote_type(Int, Float64). Problems seem to arise when Core.TypeofBottom is involved...

tkelman · 2017-05-13T07:09:58Z

you appear to have added an empty file called cd by mistake here

andyferris · 2017-05-15T11:12:42Z

OK, I really have been stretched for time since I first looked at this, but everytime I have a look something confuses me.

My current dilemma is that inference seems to do a poor job for promote for some reason, despite all the functions it calls being fully inferable. For example:

@code_warntype promote(1.0, 1)
Variables:
  #self#::Base.#promote
  x::Float64
  y::Int64

Body:
  begin  # line 175:
      return (Core.tuple)(x::Float64, (Base.sitofp)(Float64, y::Int64)::Float64)::Tuple{Any,Any}
  end::Tuple{Any,Any}

I've never seen tuple fail in inference before, so this seems special. Help appreciated! (edit: especially if there are some caveats that I should know about regarding functions defined in bootstrap, or the inferability of Bottom and TypeofBottom).

martinholters · 2017-05-16T09:08:46Z

May or may not be related, but observed while looking into this:

julia> @code_warntype promote_type(Int,Union{})
Variables:
  #self#::Base.#promote_type
  #unused#@_2::Any
  #unused#@_3::Any

Body:
  begin 
      return $(Expr(:static_parameter, 1))
  end::Type{Int64}

julia> @code_warntype promote_type(Int,Union{})
Variables:
  #temp#@_1
  #temp#@_2
  #temp#@_3

Body:
  begin 
      return $(QuoteNode(Int64))
  end::Type{Int64}

Any ideas anyone why the output changes upon second invocation? (Stays that way thereafter.)

andyferris · 2017-05-16T09:47:47Z

That is interesting. Possibly related to the abused @_pure_meta in promote_rule?

Anyway we certainly are seeing a range of behaviours - ideally we:

fix constant propagation of Union{}.
understand how inference could have failed on the tuple I gave earlier?
make sure there is no world confusion from remaining pure annotations, such as @martinholters's example.

martinholters · 2017-05-17T06:27:58Z

Regarding my comment, it has nothing to do with pure annotations, and occurs also outside the promotion stuff:

julia> foo(::Type{T}) where {T} = T
foo (generic function with 1 method)

julia> @code_warntype foo(Int)
Variables:
  #self#::#foo
  #unused#::Any

Body:
  begin 
      return $(Expr(:static_parameter, 1))
  end::Type{Int64}

julia> @code_warntype foo(Int)
Variables:
  #temp#@_1
  #temp#@_2

Body:
  begin 
      return $(QuoteNode(Int64))
  end::Type{Int64}

So, interesting, but probably unrelated.

andyferris · 2017-05-17T06:35:38Z

OK, that is interesting...

martinholters · 2017-05-19T11:47:03Z

This goes a long way:

--- a/base/inference.jl
+++ b/base/inference.jl
@@ -1842,7 +1842,7 @@ function abstract_call(f::ANY, fargs::Union{Tuple{},Vector{Any}}, argtypes::V
ect
     t = pure_eval_call(f, argtypes, atype, sv)
     t !== false && return t
 
-    if istopfunction(tm, f, :promote_type) || istopfunction(tm, f, :typejoin)
+    if istopfunction(tm, f, :typejoin)
         return Type
     elseif length(argtypes) == 2 && istopfunction(tm, f, :typename)
         return typename_static(argtypes[2])

I have no idea what purpose this served, though... There are more mentions of the functions you touch here in inference.jl which I guess should all be revisited.

vtjnash · 2017-05-22T21:09:58Z

I have no idea what purpose this served, though

It's avoiding inferring over those functions, since it figures if pure-eval failed, there's no point in computing (and caching) an inferred version of these functions – especially if they may get big, but have no actual useful answer. Definitely a good patch you found. With it, I had just one remaining failure (in nullable). The return_type tfunc is asking a malformed question (the return type of a function that can't be called is...?), but we should be handling that better.

KristofferC · 2017-05-22T21:11:42Z

@nanosoldier runbenchmarks(ALL, vs = ":master")

jrevels · 2017-05-22T21:28:06Z

Kicked the server:

@nanosoldier runbenchmarks(ALL, vs = ":master")

andyferris · 2017-05-22T23:55:36Z

base/promotion.jl

@@ -161,26 +161,30 @@ function promote_type(::Type{T}, ::Type{S}) where {T,S}
    promote_result(T, S, promote_rule(T,S), promote_rule(S,T))
 end

-promote_rule(T, S) = (@_pure_meta; Bottom)
+# TODO: this is not hyperpure, but the pure anotation seems necessary for inference to work
+promote_rule(::Type{<:Any}, ::Type{<:Any}) = (@_pure_meta; Bottom)


I guess we still need to remove the @_pure_meta from here...

andyferris · 2017-05-22T23:57:33Z

Cool. :) There's still some @_pure_metas hanging around whenever Bottom is returned - will these be handled better now without the pure annotation?

nanosoldier · 2017-05-23T00:28:37Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

vtjnash · 2017-05-24T18:51:38Z

Can't hurt to find out :)

@nanosoldier runbenchmarks(ALL, vs = ":master")

nanosoldier · 2017-05-24T21:45:08Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

andyferris · 2017-05-24T21:47:03Z

Nice. Nanosoldier regressions look like noise? (If I'm reading the error bars correctly?)

We do need to backport this to v0.6 - the resulting bugs from world age/pure interactions have been found in the wild, as mentioned in the OP. (@tkelman?)

vtjnash · 2017-05-25T00:27:41Z

nice indeed. should be good to merge.

tkelman · 2017-05-25T01:37:12Z

would intermediate commits pass tests? if not, should be squashed

martinholters · 2017-05-26T06:47:40Z

base/promotion.jl

@@ -161,26 +161,30 @@ function promote_type(::Type{T}, ::Type{S}) where {T,S}
    promote_result(T, S, promote_rule(T,S), promote_rule(S,T))
 end

-promote_rule(T, S) = (@_pure_meta; Bottom)
+# TODO: this is not hyperpure, but the pure anotation seems necessary for inference to work


This comment is obsolete now, it seems.

removes performance bug added by 0292c42

this allows printing of the Exprs flowing through inference (which might instead have the field set to something like a Const object)

Removing actually may enable inference to get a sharper result, since it is no longer being directed to ignore backedges and correctness assumptions Replaces pure annotations in promotion with inline

GunnarFarneback · 2017-05-30T09:13:56Z

There's still something amiss here.

julia> versioninfo()
Julia Version 0.7.0-DEV.391
Commit 526ed78 (2017-05-30 03:28 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Haswell)
  LAPACK: libopenblas64_
  LIBM: libopenlibm
  LLVM: libLLVM-3.9.1 (ORCJIT, haswell)

julia> promote_rule(::Type{Float64}, ::Type{Float32}) = Float32
promote_rule (generic function with 1 method)

julia> promote_type(Float64, Float32)
Float64

julia> function Base.promote_type(::Type{T}, ::Type{S}) where {T,S}
           Base.@_inline_meta
           # Try promote_rule in both orders. Typically only one is defined,
           # and there is a fallback returning Bottom below, so the common case is
           #   promote_type(T, S) =>
           #   promote_result(T, S, result, Bottom) =>
           #   typejoin(result, Bottom) => result
           promote_result(T, S, promote_rule(T,S), promote_rule(S,T))
       end
WARNING: Method definition promote_type(Type{T}, Type{S}) in module Base at promotion.jl:155 overwritten in module Main at REPL[4]:2.

fatal: error thrown and no exception handler available.
UndefVarError(var=:promote_result)
rec_backtrace at /home/gunnar/julia0.7/src/stackwalk.c:84
record_backtrace at /home/gunnar/julia0.7/src/task.c:245
jl_throw at /home/gunnar/julia0.7/src/task.c:564
jl_undefined_var_error at /home/gunnar/julia0.7/src/rtutils.c:129
jl_get_binding_or_error at /home/gunnar/julia0.7/src/module.c:236
== at ./int.jl:295
datatype_pointerfree at ./reflection.jl:193
isbits at ./reflection.jl:233
vcat at ./array.jl:1118
jl_call_fptr_internal at /home/gunnar/julia0.7/src/julia_internal.h:353 [inlined]
jl_call_method_internal at /home/gunnar/julia0.7/src/julia_internal.h:372 [inlined]
jl_apply_generic at /home/gunnar/julia0.7/src/gf.c:1923
jl_apply at /home/gunnar/julia0.7/src/julia.h:1424 [inlined]
jl_f__apply at /home/gunnar/julia0.7/src/builtins.c:426
stacktrace at ./stacktraces.jl:151
display_error at ./client.jl:128
unknown function (ip: 0x7f814c06c83d)
jl_call_fptr_internal at /home/gunnar/julia0.7/src/julia_internal.h:353 [inlined]
jl_call_method_internal at /home/gunnar/julia0.7/src/julia_internal.h:372 [inlined]
jl_apply_generic at /home/gunnar/julia0.7/src/gf.c:1923
display_error at ./client.jl:142
unknown function (ip: 0x7f814c06c2c6)
jl_call_fptr_internal at /home/gunnar/julia0.7/src/julia_internal.h:353 [inlined]
jl_call_method_internal at /home/gunnar/julia0.7/src/julia_internal.h:372 [inlined]
jl_apply_generic at /home/gunnar/julia0.7/src/gf.c:1923
do_call at /home/gunnar/julia0.7/src/interpreter.c:75
eval at /home/gunnar/julia0.7/src/interpreter.c:242
eval_body at /home/gunnar/julia0.7/src/interpreter.c:539
jl_toplevel_eval_body at /home/gunnar/julia0.7/src/interpreter.c:511
jl_toplevel_eval_flex at /home/gunnar/julia0.7/src/toplevel.c:569 [inlined]
jl_toplevel_eval at /home/gunnar/julia0.7/src/toplevel.c:598
jl_toplevel_eval_in at /home/gunnar/julia0.7/src/builtins.c:496
eval at ./boot.jl:236
unknown function (ip: 0x7f816625370f)
jl_call_fptr_internal at /home/gunnar/julia0.7/src/julia_internal.h:353 [inlined]
jl_call_method_internal at /home/gunnar/julia0.7/src/julia_internal.h:372 [inlined]
jl_apply_generic at /home/gunnar/julia0.7/src/gf.c:1923
_start at ./client.jl:419
unknown function (ip: 0x7f81662a8ea8)
jl_call_fptr_internal at /home/gunnar/julia0.7/src/julia_internal.h:353 [inlined]
jl_call_method_internal at /home/gunnar/julia0.7/src/julia_internal.h:372 [inlined]
jl_apply_generic at /home/gunnar/julia0.7/src/gf.c:1923
jl_apply at /home/gunnar/julia0.7/ui/../src/julia.h:1424 [inlined]
true_main at /home/gunnar/julia0.7/ui/repl.c:127
main at /home/gunnar/julia0.7/ui/repl.c:264
__libc_start_main at /build/eglibc-MjiXCM/eglibc-2.19/csu/libc-start.c:287
unknown function (ip: 0x4015fa)

martinholters · 2017-05-30T09:20:23Z

IIUC you're redefining Base.promote_type in terms of Main.promote_result, which is undefined. I.e. Base.promote_type can now be expected to fail, and as it used so widespread, it's no wonder this causes an ungraceful exit. If this used to work (did it?), then because all the @pures prevented your new definition from actually taking full effect.

GunnarFarneback · 2017-05-30T09:37:27Z

Right, I missed to specify Base when I tried to change the promote_rule. This now works fine. :-)

julia> 1.0 * 1.0f0
1.0

julia> 1.0f0 * 1.0
1.0

julia> Base.promote_rule(::Type{Float64}, ::Type{Float32}) = Float32
WARNING: Method definition promote_rule(Type{Float64}, Type{Float32}) in module Base at float.jl:360 overwritten in module Main at REPL[2]:1.

julia> 1.0f0 *1.0
1.0f0

julia> 1.0 * 1.0f0
1.0f0

The redefinition that caused the crash was an attempt to work around my first mistake, so I have no actual interest in that part, although a less ungraceful exit would be, well, more graceful.

andyferris · 2017-05-30T23:12:56Z

This really needs a "backport to v0.6" tag, please.

(cherry picked from commit 15e7369) ref #21771 remove many unneeded pure annotations Removing actually may enable inference to get a sharper result, since it is no longer being directed to ignore backedges and correctness assumptions Replaces pure annotations in promotion with inline (cherry picked from commit 76a30fb) handling Base printing of Expr.typ fields not containing a Type this allows printing of the Exprs flowing through inference (which might instead have the field set to something like a Const object) (cherry picked from commit f97d9e8) apply_type on Type{T} is valid whenever T is valid removes performance bug added by 0292c42 (cherry picked from commit 8ee2e06)

tkelman · 2017-06-06T00:04:24Z

This caused a test failure in Unitful.jl, cc @ajkeller34

This reverts commit 456198f, the backport of #21771, because it caused a regression in Unitful.jl

andyferris · 2017-06-06T01:56:19Z

Where is the Unitful error?

tkelman · 2017-06-06T01:57:24Z

https://gist.github.com/anonymous/62d8d15eee91ce2332322a54fee0bffa#file-pkgeval_unitful_test-log

ararslan requested a review from vtjnash May 10, 2017 20:33

JeffBezanson reviewed May 10, 2017

View reviewed changes

kshyatt added the types and dispatch Types, subtyping and method dispatch label May 11, 2017

andyferris force-pushed the ajf/remove-pure-from-promotion branch from e95afce to b0d3f2e Compare May 13, 2017 06:16

andyferris force-pushed the ajf/remove-pure-from-promotion branch from b0d3f2e to 3f11961 Compare May 13, 2017 07:05

andyferris force-pushed the ajf/remove-pure-from-promotion branch 3 times, most recently from 42a5458 to c2db97b Compare May 16, 2017 01:00

vtjnash force-pushed the ajf/remove-pure-from-promotion branch 2 times, most recently from c4b9875 to 0c5637d Compare May 22, 2017 21:09

vtjnash approved these changes May 22, 2017

View reviewed changes

andyferris commented May 22, 2017

View reviewed changes

vtjnash force-pushed the ajf/remove-pure-from-promotion branch from 0c5637d to cb39d4c Compare May 24, 2017 18:51

martinholters reviewed May 26, 2017

View reviewed changes

vtjnash force-pushed the ajf/remove-pure-from-promotion branch from cb39d4c to 5001c6d Compare May 26, 2017 15:48

vtjnash changed the title ~~RFC/WIP Replace pure annotations in promotion with inline~~ eplace pure annotations in promotion with inline May 26, 2017

vtjnash and others added 4 commits May 29, 2017 11:10

improve inference of methods with Type{leaftype} parameters

15e7369

apply_type on Type{T} is valid whenever T is valid

8ee2e06

removes performance bug added by 0292c42

handling Base printing of Expr.typ fields not containing a Type

f97d9e8

this allows printing of the Exprs flowing through inference (which might instead have the field set to something like a Const object)

remove many unneeded pure annotations

76a30fb

Removing actually may enable inference to get a sharper result, since it is no longer being directed to ignore backedges and correctness assumptions Replaces pure annotations in promotion with inline

vtjnash force-pushed the ajf/remove-pure-from-promotion branch from 5001c6d to 76a30fb Compare May 29, 2017 15:10

vtjnash changed the title ~~eplace pure annotations in promotion with inline~~ replace pure annotations in promotion with inline May 29, 2017

vtjnash merged commit 75541d2 into JuliaLang:master May 29, 2017

This was referenced May 30, 2017

Ungraceful exit when redefining Base.promote_type #22132

Closed

Inconsistent behavior when extending an operator without first importing it. #2071

Closed

andyferris deleted the ajf/remove-pure-from-promotion branch May 30, 2017 23:13

StefanKarpinski added the backport pending 0.6 label May 30, 2017

tkelman mentioned this pull request Jun 5, 2017

[release-0.6] backports for 0.6.0-rc3 #22199

Merged

3 tasks

tkelman added a commit that referenced this pull request Jun 6, 2017

Revert "improve inference of methods with Type{leaftype} parameters"

e531c84

This reverts commit 456198f, the backport of #21771, because it caused a regression in Unitful.jl

tkelman removed the backport pending 0.6 label Jun 7, 2017

andyferris mentioned this pull request Jun 15, 2017

Calling promote_tuple_eltype() causes promote_type to change behavior JuliaArrays/StaticArrays.jl#228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace pure annotations in promotion with inline #21771

replace pure annotations in promotion with inline #21771

andyferris commented May 10, 2017

KristofferC commented May 10, 2017

nanosoldier commented May 10, 2017

vtjnash commented May 10, 2017

JeffBezanson May 10, 2017

andyferris May 11, 2017

andyferris commented May 13, 2017 •

edited

Loading

tkelman commented May 13, 2017

andyferris commented May 15, 2017 •

edited

Loading

martinholters commented May 16, 2017

andyferris commented May 16, 2017 •

edited

Loading

martinholters commented May 17, 2017

andyferris commented May 17, 2017

martinholters commented May 19, 2017

vtjnash commented May 22, 2017

KristofferC commented May 22, 2017

jrevels commented May 22, 2017

andyferris May 22, 2017

andyferris commented May 22, 2017

nanosoldier commented May 23, 2017

vtjnash commented May 24, 2017

nanosoldier commented May 24, 2017

andyferris commented May 24, 2017

vtjnash commented May 25, 2017

tkelman commented May 25, 2017

martinholters May 26, 2017

GunnarFarneback commented May 30, 2017

martinholters commented May 30, 2017

GunnarFarneback commented May 30, 2017

andyferris commented May 30, 2017

tkelman commented Jun 6, 2017

andyferris commented Jun 6, 2017

tkelman commented Jun 6, 2017

replace pure annotations in promotion with inline #21771

replace pure annotations in promotion with inline #21771

Conversation

andyferris commented May 10, 2017

KristofferC commented May 10, 2017

nanosoldier commented May 10, 2017

vtjnash commented May 10, 2017

JeffBezanson May 10, 2017

Choose a reason for hiding this comment

andyferris May 11, 2017

Choose a reason for hiding this comment

andyferris commented May 13, 2017 • edited Loading

tkelman commented May 13, 2017

andyferris commented May 15, 2017 • edited Loading

martinholters commented May 16, 2017

andyferris commented May 16, 2017 • edited Loading

martinholters commented May 17, 2017

andyferris commented May 17, 2017

martinholters commented May 19, 2017

vtjnash commented May 22, 2017

KristofferC commented May 22, 2017

jrevels commented May 22, 2017

andyferris May 22, 2017

Choose a reason for hiding this comment

andyferris commented May 22, 2017

nanosoldier commented May 23, 2017

vtjnash commented May 24, 2017

nanosoldier commented May 24, 2017

andyferris commented May 24, 2017

vtjnash commented May 25, 2017

tkelman commented May 25, 2017

martinholters May 26, 2017

Choose a reason for hiding this comment

GunnarFarneback commented May 30, 2017

martinholters commented May 30, 2017

GunnarFarneback commented May 30, 2017

andyferris commented May 30, 2017

tkelman commented Jun 6, 2017

andyferris commented Jun 6, 2017

tkelman commented Jun 6, 2017

andyferris commented May 13, 2017 •

edited

Loading

andyferris commented May 15, 2017 •

edited

Loading

andyferris commented May 16, 2017 •

edited

Loading